Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Truly Dynamic Event #4274

Merged
merged 7 commits into from
Jul 9, 2024
Merged

Conversation

cshung
Copy link
Member

@cshung cshung commented Jun 18, 2024

This PR is currently in a prototype state. (It is easier to review by commits)

This change:

  1. Upgrade TraceEvent dependencies from 3.1.7 to 3.1.9, which also require upgrading the project to 8.0, so that we can
  2. Allow using custom TraceEvent library, built some support for using it, to
  3. Parse DynamicEvent and allow them to be accessed in the Notebooks.

And some other cleanups

  1. Run dotnet-format on the source code
  2. Ignore xlf files
  3. Deleted unwanted (and failing) outputs from Notebook examples.

The dynamic event consumption part can only work with microsoft/perfview#2051.

@cshung cshung force-pushed the public/truly-dynamic-event branch from 6cd369d to ec7ff57 Compare June 18, 2024 16:55
@cshung cshung force-pushed the public/truly-dynamic-event branch 2 times, most recently from c07614a to 4c3e7d6 Compare June 19, 2024 14:40
@cshung cshung force-pushed the public/truly-dynamic-event branch 3 times, most recently from bcd4c83 to ef37cb3 Compare June 21, 2024 20:30
@cshung cshung force-pushed the public/truly-dynamic-event branch 5 times, most recently from 5e08c1b to 0b722d2 Compare July 1, 2024 22:26
mrsharm
mrsharm previously approved these changes Jul 2, 2024
Copy link
Member

@mrsharm mrsharm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - just some nits here and there but we tested it out and it all works.

@cshung cshung force-pushed the public/truly-dynamic-event branch from e06b946 to e46e782 Compare July 2, 2024 04:09
Copy link
Member

@mrsharm mrsharm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@mrsharm
Copy link
Member

mrsharm commented Jul 3, 2024

@cincuranet - could we please get this PR merged, seems like the Build Analysis failures are unrelated:

❌The pipeline is not valid. Job windows_22H2_x64_scenarios__Open: Step AzureCLI input connectedServiceNameARM references service connection .NET Performance (790c4451-dad9-4fda-af8b-10bd9ca328fa) which could not be found. The service connection does not exist, has been disabled or has not been authorized for use. For authorization details, refer to https://aka.ms/yamlauthz. Job ubuntu_2204_x64_scenarios__Open: Step AzureCLI input connectedServiceNameARM references service connection .NET Performance (790c4451-dad9-4fda-af8b-10bd9ca328fa) which could not be found. The service connection does not exist, has been disabled or has not been authorized for use. For authorization details, refer to https://aka.ms/yamlauthz.

Thanks!

@cincuranet
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@cincuranet
Copy link
Contributor

@LoopedBard3
Copy link
Member

Error was fixed earlier this week and this PR is approved by correct people, merging 👍.

@LoopedBard3 LoopedBard3 merged commit 252ebaa into dotnet:main Jul 9, 2024
45 of 63 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants